Conversation
📝 WalkthroughWalkthroughA new integration test for GitHub Checks was introduced, along with supporting configuration updates. The CI workflow now supports an optional manual job for running these integration tests. Jest configuration was updated to include a global setup file for polyfilling fetch with undici. Existing GitHub Checks tests were refactored to always run with mocked API calls. A new test helper module was added to facilitate parameter creation with overrides. Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant GitHub Actions
participant Integration Test
participant GitHub API
Developer->>GitHub Actions: Manually triggers workflow_dispatch with runGithubIntegrationTests=true
GitHub Actions->>Integration Test: Starts githubChecksIntegration job
Integration Test->>GitHub API: Creates GitHub check (real API call)
Integration Test->>GitHub API: Updates GitHub check (real API call)
Integration Test->>GitHub API: Updates GitHub check (real API call)
Integration Test-->>GitHub Actions: Reports test results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
|
|
Was unable to push to this branch with codex unfortunately, sorry had to recreate this time @cloudymax
• Why is it needed to disable certain ESLint options? — The only disables are for camelcase where the GitHub API requires snake_case fields, so we suppress the camelCase rule around those API payload keys. • Could mocking the GitHub return values lead to tests passing erroneously if the GitHub API changes? — Mocking keeps unit tests deterministic and offline. To catch breaking API changes, we added an optional integration test (RUN_GITHUB_INTEGRATION_TESTS) that exercises real GitHub checks, providing a safety net for API regressions. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/jest.globals.ts (1)
1-3: LGTM with minor suggestions for TypeScript compatibility.The fetch polyfill implementation is correct. The ESLint error about
globalThisis a false positive -globalThisis a standard global object in Node.js 12+ and modern browsers.Consider adding TypeScript declarations to improve type safety:
import { fetch as undiciFetch, Headers, Request, Response } from 'undici'; +declare global { + var fetch: typeof undiciFetch; + var Headers: typeof Headers; + var Request: typeof Request; + var Response: typeof Response; +} + Object.assign(globalThis, { fetch: undiciFetch, Headers, Request, Response });src/model/cloud-runner/tests/cloud-runner-github-checks.test.ts (1)
18-33: LGTM! Good refactoring to mock GitHub API calls.The addition of mocking in
beforeEach/afterEachhooks properly isolates unit tests from external dependencies. This is the correct approach for unit testing.Address the ESLint warning by simplifying the undefined return:
- jest.spyOn(GitHub as any, 'runUpdateAsyncChecksWorkflow').mockResolvedValue(undefined); + jest.spyOn(GitHub as any, 'runUpdateAsyncChecksWorkflow').mockResolvedValue();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/cloud-runner-ci-pipeline.yml(2 hunks)jest.config.js(1 hunks)src/integration/cloud-runner-github-checks-integration-test.ts(1 hunks)src/jest.globals.ts(1 hunks)src/model/cloud-runner/tests/cloud-runner-github-checks.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: configure code analysis tools to recognize typescript files to ensure accurate detection of properti...
Learnt from: ejnarvala
PR: game-ci/unity-builder#661
File: src/model/platform-setup.ts:35-35
Timestamp: 2024-10-25T18:45:10.112Z
Learning: Configure code analysis tools to recognize TypeScript files to ensure accurate detection of properties like `unityLicensingProductIds` in future reviews.
Applied to files:
jest.config.js
📚 Learning: the build steps duplication in the workflow is intentional to implement a retry mechanism with diffe...
Learnt from: MichaelBuhler
PR: game-ci/unity-builder#685
File: .github/workflows/build-tests-ubuntu.yml:146-146
Timestamp: 2025-01-30T18:01:50.339Z
Learning: The build steps duplication in the workflow is intentional to implement a retry mechanism with different delay intervals between attempts.
Applied to files:
.github/workflows/cloud-runner-ci-pipeline.yml
🧬 Code Graph Analysis (1)
src/integration/cloud-runner-github-checks-integration-test.ts (3)
src/model/cloud-runner/tests/cloud-runner-github-checks.test.ts (1)
TIMEOUT_INFINITE(8-8)src/model/cli/cli.ts (1)
Cli(16-175)src/model/unity-versioning.ts (1)
UnityVersioning(4-32)
🪛 ESLint
src/jest.globals.ts
[error] 3-3: 'globalThis' is not defined.
(no-undef)
src/model/cloud-runner/tests/cloud-runner-github-checks.test.ts
[error] 28-28: Do not use useless undefined.
(unicorn/no-useless-undefined)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: StandaloneLinux64 on 2023.2.2f1
- GitHub Check: WebGL on 6000.0.36f1 (via Build Profile)
- GitHub Check: WebGL on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: Android on 2023.2.2f1
- GitHub Check: iOS on 2023.2.2f1
- GitHub Check: StandaloneLinux64 on 2023.2.2f1
- GitHub Check: Android on 2022.3.13f1
- GitHub Check: WebGL on 2023.2.2f1
- GitHub Check: StandaloneLinux64 on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 2022.3.13f1
- GitHub Check: StandaloneLinux64 on 2022.3.13f1
- GitHub Check: WebGL on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneOSX on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: Tests
🔇 Additional comments (7)
jest.config.js (1)
28-32: LGTM! Clean Jest configuration update.The addition of
setupFilesto load the fetch polyfill before each test file is correct, and the updated comments clearly distinguish betweensetupFilesandsetupFilesAfterEnv.src/model/cloud-runner/tests/cloud-runner-github-checks.test.ts (1)
35-72: LGTM! Tests properly exercise both direct and async GitHub check workflows.The tests correctly cover both direct GitHub check creation/updates and async workflow scenarios. The infinite timeout is appropriate for integration-style tests that may take time to complete.
src/integration/cloud-runner-github-checks-integration-test.ts (3)
17-19: LGTM! Clean conditional test execution pattern.The use of
describeOrSkipbased on the environment variable is a clean way to conditionally run integration tests.
32-38: LGTM! Integration test properly exercises real GitHub API calls.The test correctly creates and updates GitHub checks with realistic scenarios, including proper assertions and status transitions.
24-31: Test project directory and version file verified
Confirmed thattest-projectandtest-project/ProjectSettings/ProjectVersion.txtboth exist in the CI environment and contain a valid Unity editor version. No further changes are needed..github/workflows/cloud-runner-ci-pipeline.yml (2)
6-10: LGTM! Clean workflow input parameter for optional integration tests.The addition of
runGithubIntegrationTestsinput with proper defaults and description follows GitHub Actions best practices.
216-231: LGTM! Well-structured conditional integration test job.The job correctly:
- Runs only on manual workflow dispatch when explicitly requested
- Sets appropriate environment variables
- Uses proper Node.js setup with caching
- Runs tests with appropriate Jest flags for integration testing
- Passes required secrets
| export const TIMEOUT_INFINITE = 1e9; | ||
|
|
||
| async function createParameters(overrides?: OptionValues) { | ||
| if (overrides) Cli.options = overrides; | ||
|
|
||
| return BuildParameters.create(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address code duplication with the unit test file.
Both TIMEOUT_INFINITE and createParameters are duplicated from src/model/cloud-runner/tests/cloud-runner-github-checks.test.ts. Consider extracting these to a shared test utilities module.
Create a shared utilities file like src/test-utils/cloud-runner-test-helpers.ts:
import { BuildParameters } from '../model';
import { Cli } from '../model/cli/cli';
import { OptionValues } from 'commander';
export const TIMEOUT_INFINITE = 1e9;
export async function createParameters(overrides?: OptionValues) {
if (overrides) Cli.options = overrides;
return BuildParameters.create();
}Then import from both test files.
🤖 Prompt for AI Agents
In src/integration/cloud-runner-github-checks-integration-test.ts around lines 9
to 15, the constants TIMEOUT_INFINITE and the function createParameters are
duplicated from another test file. To fix this, extract both into a new shared
test utility module, for example src/test-utils/cloud-runner-test-helpers.ts,
and move the definitions there. Then update this test file and the other test
file to import TIMEOUT_INFINITE and createParameters from the new shared module
instead of defining them locally.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/test-utils/cloud-runner-test-helpers.ts (1)
9-9: Add blank line before return statement.ESLint requires a blank line before the return statement for better readability.
if (overrides) Cli.options = overrides; + return BuildParameters.create();src/model/cloud-runner/tests/cloud-runner-github-checks.test.ts (1)
20-20: Remove unnecessary undefined value.ESLint flags the explicit
undefinedas unnecessary sincemockResolvedValue()without arguments defaults to undefined.- jest.spyOn(GitHub as any, 'runUpdateAsyncChecksWorkflow').mockResolvedValue(undefined); + jest.spyOn(GitHub as any, 'runUpdateAsyncChecksWorkflow').mockResolvedValue();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
jest.config.js(1 hunks)src/integration/cloud-runner-github-checks-integration-test.ts(1 hunks)src/model/cloud-runner/tests/cloud-runner-github-checks.test.ts(1 hunks)src/test-utils/cloud-runner-test-helpers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- jest.config.js
- src/integration/cloud-runner-github-checks-integration-test.ts
🧰 Additional context used
🪛 ESLint
src/model/cloud-runner/tests/cloud-runner-github-checks.test.ts
[error] 20-20: Do not use useless undefined.
(unicorn/no-useless-undefined)
src/test-utils/cloud-runner-test-helpers.ts
[error] 9-9: Expected blank line before this statement.
(padding-line-between-statements)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: StandaloneOSX on 2022.3.13f1
- GitHub Check: StandaloneLinux64 on 2022.3.13f1
- GitHub Check: StandaloneOSX on 2023.2.2f1
- GitHub Check: StandaloneOSX on 2022.3.13f1
- GitHub Check: iOS on 2022.3.13f1
- GitHub Check: StandaloneOSX on 2021.3.45f1
- GitHub Check: iOS on 2021.3.45f1
- GitHub Check: iOS on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneOSX on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: WSAPlayer on 2023.2.2f1
- GitHub Check: StandaloneWindows64 on 2022.3.13f1
- GitHub Check: tvOS on 2023.2.2f1
- GitHub Check: Android on 2023.2.2f1
- GitHub Check: tvOS on 2021.3.32f1
- GitHub Check: Android on 2022.3.13f1
- GitHub Check: Android on 2021.3.32f1
🔇 Additional comments (4)
src/test-utils/cloud-runner-test-helpers.ts (1)
5-5: LGTM!The
TIMEOUT_INFINITEconstant provides a clear, reusable timeout value for long-running tests.src/model/cloud-runner/tests/cloud-runner-github-checks.test.ts (3)
10-25: Excellent mocking strategy for test isolation.The beforeEach/afterEach pattern properly isolates tests from external GitHub API dependencies while ensuring clean state between tests. This aligns perfectly with the PR objective of improving test reliability.
31-37: LGTM on parameter setup standardization.Using the shared
createParametershelper improves consistency across tests and eliminates code duplication. The parameter configuration properly enables GitHub checks and async workflow for testing.Also applies to: 49-55
27-44: Well-structured test cases with appropriate timeout.Both test cases follow a clear pattern:
- Setup parameters with required configuration
- Initialize CloudRunner
- Exercise GitHub check creation and updates
- Use infinite timeout for potentially long-running operations
The tests effectively validate both direct and async workflow scenarios as intended.
Also applies to: 45-64
| export async function createParameters(overrides?: OptionValues) { | ||
| if (overrides) Cli.options = overrides; | ||
| return BuildParameters.create(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider test isolation concerns with global state mutation.
The function modifies Cli.options globally without cleanup, which could cause test isolation issues if multiple tests run concurrently or if a test fails before cleanup.
Consider one of these approaches:
- Return a cleanup function or use try/finally
- Store the original options and restore them
- Make the function more explicit about its side effects
export async function createParameters(overrides?: OptionValues) {
+ const originalOptions = Cli.options;
if (overrides) Cli.options = overrides;
- return BuildParameters.create();
+ try {
+ return await BuildParameters.create();
+ } finally {
+ Cli.options = originalOptions;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function createParameters(overrides?: OptionValues) { | |
| if (overrides) Cli.options = overrides; | |
| return BuildParameters.create(); | |
| } | |
| export async function createParameters(overrides?: OptionValues) { | |
| const originalOptions = Cli.options; | |
| if (overrides) Cli.options = overrides; | |
| try { | |
| return await BuildParameters.create(); | |
| } finally { | |
| Cli.options = originalOptions; | |
| } | |
| } |
🧰 Tools
🪛 ESLint
[error] 9-9: Expected blank line before this statement.
(padding-line-between-statements)
🤖 Prompt for AI Agents
In src/test-utils/cloud-runner-test-helpers.ts around lines 7 to 10, the
function createParameters modifies the global Cli.options without restoring it,
risking test isolation issues. To fix this, store the original Cli.options value
before overwriting it, then restore the original value after
BuildParameters.create() completes, using a try/finally block or returning a
cleanup function to ensure the global state is reset regardless of test
outcomes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #724 +/- ##
==========================================
+ Coverage 37.05% 38.34% +1.28%
==========================================
Files 77 78 +1
Lines 3163 3169 +6
Branches 626 663 +37
==========================================
+ Hits 1172 1215 +43
+ Misses 1991 1809 -182
- Partials 0 145 +145
🚀 New features to boost your workflow:
|
frostebite
deleted the
fnkp48-codex/fix-fetch-implementation-issue-in-tests
branch
|
Released in v4.6.0 |
3 participants
Improving stability/fix for occasional fail on a test for coordinator/cloud runner.
Mocks a github check API which very occasionally fails.
Summary
Testing
npx prettier src/integration/cloud-runner-github-checks-integration-test.ts .github/workflows/cloud-runner-ci-pipeline.yml src/jest.setup.ts src/jest.globals.ts src/model/cloud-runner/tests/cloud-runner-github-checks.test.ts jest.config.js --checknpx eslint src/**/*.ts(fails: ESLint couldn't find the @typescript-eslint/eslint-plugin package)cloudRunnerTests=true yarn run test "cloud-runner-github-checks" --detectOpenHandles --forceExit --runInBandhttps://chatgpt.com/codex/tasks/task_e_688e58da43948324b6030b044d3e3ec9
Summary by CodeRabbit
New Features
Tests
Chores